Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add session storage clustering support #3555

Merged
merged 14 commits into from
Nov 22, 2024

Conversation

woutslakhorst
Copy link
Member

@woutslakhorst woutslakhorst commented Nov 11, 2024

closes #3553

  • adds Memcached and Redis as options for session storage (Redis was already added but broken)
  • added e2e tests for memcached and Redis sentinel

@woutslakhorst woutslakhorst force-pushed the feature/3553/clustered_memstore branch from 586e3c9 to 88cdf94 Compare November 11, 2024 14:19
@woutslakhorst woutslakhorst force-pushed the feature/3553/clustered_memstore branch from 88cdf94 to 39aa9aa Compare November 11, 2024 14:25
@woutslakhorst woutslakhorst marked this pull request as ready for review November 18, 2024 08:52
@woutslakhorst woutslakhorst changed the title Feature/3553/clustered memstore Add session storage clustering support Nov 18, 2024
Copy link
Member

@reinkrul reinkrul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the clustering e2e-test, maybe you can spin up a second nuts node (same organization) and run the test against both (round robin)?

README.rst Show resolved Hide resolved
docs/pages/deployment/clustering.rst Outdated Show resolved Hide resolved
##########

With the introduction of a SQL database and separate session storage, clustering with HA is now possible.
Clustering is currently limited to nodes that have the ``did:nuts`` method disabled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe state what NOT to do (use default on-disk/in-memory stores for private keys and stuff)?

docs/pages/deployment/storage.rst Outdated Show resolved Hide resolved
docs/pages/deployment/storage.rst Outdated Show resolved Hide resolved
docs/pages/deployment/storage.rst Show resolved Hide resolved
e2e-tests/clustering/memcached/node-A/nginx.conf Outdated Show resolved Hide resolved
e2e-tests/clustering/redis/node-A/nginx.conf Outdated Show resolved Hide resolved
e2e-tests/clustering/redis/run-test.sh Show resolved Hide resolved
go.mod Show resolved Hide resolved
@reinkrul
Copy link
Member

Do we need/have something in diagnostics on the session store?

@woutslakhorst
Copy link
Member Author

Do we need/have something in diagnostics on the session store?

there's no ping but maybe we can do a simple Get on a random value which should return ErrNotFound.

@woutslakhorst
Copy link
Member Author

The ErrorSessionDatabase type for testing will also be used in access token caching

results["session"] = core.Health{
Status: core.HealthStatusUp,
}
err := e.sessionDatabase.GetStore(defaultSessionDataTTL, "healthcheck").Get("does_not_exist", nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope so :-p

// StringOrBytes is a type that can be either a string or a byte slice
// used for generic type constraints
type StringOrBytes interface {
~string | ~[]byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow... is this Go?


// NewMemcachedSessionDatabase creates a new MemcachedSessionDatabase using an initialized memcache.Client.
func NewMemcachedSessionDatabase(client *memcache.Client) *MemcachedSessionDatabase {
memcachedStore := memcachestore.NewMemcache(client, store.WithExpiration(defaultSessionDataTTL))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use the same value for access token lifetime? Or at least, the token lifetime shouldn't be longer than this default. Having 2 seeminly unrelated constants (but secretly relating) seems rather dangerous?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment. Memcached requires a default setting. We override this value when writing with the TTL of the store.

@woutslakhorst woutslakhorst merged commit 18f72f1 into master Nov 22, 2024
8 of 9 checks passed
@woutslakhorst woutslakhorst deleted the feature/3553/clustered_memstore branch November 22, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement clustered memory store
2 participants